-
Notifications
You must be signed in to change notification settings - Fork 64
GYE-2403: Added welsh translation toggle #158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
To ensure the language toggle follows the design guidelines here: https://design.tax.service.gov.uk/hmrc-design-patterns/welsh-language-toggle/ This what the final output looks like: Final HTML for when English is selected. And for welsh: |
|
Hi @rashadmughal I've taken a bit of a look at this. I'm quite new to the Plat UI team so apologies if my reviews aren't as quick as Jo's might have been here. I need to do a bit more investigation to understand the context of play-ui and where it pulls it's styles from before I can finish reviewing but there are a few small changes I can see would be good to make (just a couple of attributes on links, nothing big) that I'll add as comments while I get myself up to speed properly (just explaining so you're aware there there might be extra feedback after these initial comments) |
src/main/twirl/uk/gov/hmrc/play/views/layouts/LanguageSelection.scala.html
Outdated
Show resolved
Hide resolved
| </a> | ||
| </li> | ||
| } else { | ||
| <li class="hmrc-language-select__list-item" style="display: inline-block;" > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rashadmughal I'll see if I can find an alternative for you, but the use of inline style attributes here feels like something we should avoid to prevent unexpected conflicts in the future
|
@rashadmughal I've discussed this a bit more with other devs from Plat UI team and it looks like the classes you're using here depend on styles from hmrc/hmrc-frontend where as play-ui components can only rely on styles from hmrc/assets-frontend - so to create this component in play-ui it would need to be structured like the language toggle from assets-frontend https://github.com/hmrc/assets-frontend/blob/2692f3349017691e263a64142bde58d84ead4e2e/assets/components/language-selector/language-selector.html Though we'd strongly recommend that your most future proof option here is to migrate away from play-ui where possible and use hmrc/play-frontend-hmrc which already integrates a language select component https://github.com/hmrc/play-frontend-hmrc/blob/master/src/main/twirl/uk/gov/hmrc/hmrcfrontend/views/components/hmrcLanguageSelect.scala.html Though we totally understand migration is almost always simpler said than done! So Plat UI is able to help with questions and advice when migrating |
|
Hi @rashadmughal just to add to this, if you do update this PR to use assets-frontend classes to style the component from the example I linked then the other attributes (aria-label, aria-current, lang attributes, and visually hidden copy) shouldn't be removed - the example from assets-frontend doesn't have those bits (but you've got them here) and they are needed for this to meet our current accessibility standards - they're just missing from assets-frontend example because it's not been updated in quite a long time it seems! |

No description provided.